Skip to content

Conversation

@Unique-Divine
Copy link
Member

@Unique-Divine Unique-Divine commented Oct 23, 2025

Purpose / Abstract

  • fix(evmstate): trace block gas inconsistency fixed + error on trace tx failure like geth
  • Fixes [bug] Misformatted debug_TraceBlockByNumber output with callTracer #2400
  • fix(TraceBlock): use native tracer behavior, not JS parallel tracer; Make test comparisons less brittle
  • refactor: use return values without error for function that always succeeds
  • Clients won't miss failures. Downstream tooling expects JSON-RPC-compliant errors; surfacing them at the top level makes failures unambiguous and machine-detectable.
  • Parity reduces surprises. Many ecosystems (Ethereum, Base, etc.) build assumptions on geth's native tracer rules: block-level trace fails if any tx trace fails; JS-tracer "partial success" is not applicable here. Your implementation now matches those expectations.

Related

Changes

  1. TraceBlock error semantics → JSON-RPC compliant.
    debug_traceBlockByNumber now returns errors at the top level ("error": {...}, "result": null) instead of embedding an error inside the result array. This follows JSON-RPC 2.0: a failing call must include an error object and set result to null.

  2. Native-tracer parity with geth.
    Nibiru's TraceBlock mirrors geth's behavior for native tracers: iterate block txs in order, trace each, and if any trace fails, the RPC call fails. (By contrast, geth's JS tracers can aggregate per-tx errors without failing the whole call; Nibiru does not support JS tracers.) (go-ethereum])

  3. Result list construction fixed.
    TraceBlock now allocates the results slice with a length, not only capacity, preventing index-assignment panics and ensuring deterministic indexing of per-tx traces.

  4. State/log index handling clarified.
    The trace loop updates TxConfig (hash, index, and accumulated log index) per transaction so subsequent traces observe the correct evolving context, matching how geth drives tracers over a block. (Go.dev)

  5. Timeout path is tested and surfaced.
    Tests include an ultra-small tracer timeout ("1ns") to assert that a tracer stop triggers a top-level RPC error for the whole block trace—again aligning with native-tracer behavior.

  6. Cleaner signer + base-fee usage.
    MakeSigner stays aligned with block height and timestamp, and base fee units are applied consistently where needed.

  7. Naming and Sudo helpers.
    The "Sai oracle" ante decorator was generalized to AnteDecZeroGasActors and now reads from GetZeroGasActors(ctx); the querier exposes a small helper and retains the gRPC path.

Added/Updated Tests

  • Empty block (returns []).
  • Normal tracing for a single ETH transfer and an ERC-20 transfer (field-level assertions with address normalization).
  • Timeout case (forces tracer stop → top-level error).
  • TraceTx/TraceBlock JSON shape (valid JSON, correct per-tx result object extraction).

Minor DX improvements.

  • Better debug log on block fetch ({ blockHeight: ... } included).
  • Test naming (TestEvmState) and expectations updated.
  • eth_estimateGas test expectation loosened where prior behavior was too strict.

@Unique-Divine Unique-Divine requested a review from a team as a code owner October 23, 2025 13:41
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Warning

Rate limit exceeded

@Unique-Divine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 3 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between c486bde and dd9dad8.

📒 Files selected for processing (3)
  • x/evm/cli/query.go (1 hunks)
  • x/evm/evmstate/grpc_query_test.go (10 hunks)
  • x/sudo/keeper/querier.go (1 hunks)

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Fixes JSON-RPC error propagation for native tracers in debug_traceBlockByNumber, renames and reworks zero-gas actor ante decorator retrieval, adds TxHash to trace results, and updates tracing logic and tests to surface tracer timeouts and map-based trace expectations.

Changes

Cohort / File(s) Summary
Changelog
CHANGELOG.md
Added Unreleased changelog entry describing bug fix for debug_traceBlockByNumber native tracer JSON-RPC error handling.
Ante / Zero-Gas Actors
app/ante.go, app/ante/fixed_gas.go, x/sudo/keeper/querier.go
Renamed AnteDecSaiOracleAnteDecZeroGasActors; replaced runtime Sudo query with Keeper.GetZeroGasActors(ctx) getter; updated decorator registration and comments.
RPC error wrapping
eth/rpc/rpcapi/api_debug.go
Wraps tracer errors with blockHeight context using fmt.Errorf and adjusts logging/error construction in TraceBlockByNumber path.
RPC tracing flow
eth/rpc/rpcapi/tracing.go, eth/rpc/rpcapi/tracing_test.go
Clarified variable names (blockTxs/blockTx), return empty slice for zero-tx blocks, added wantErr to tests and a timeout-based error test case with conditional assertions.
gRPC TraceBlock changes
x/evm/evmstate/grpc_query.go, x/evm/evmstate/grpc_query_test.go
TraceBlock now returns []evm.TxTraceResult (value slice), treats native tracer errors as block-wide failures with formatted txHash+height error, and tests switched to map-based trace result helpers and field-by-field validation.
Trace result struct
x/evm/vmtracer.go
Added TxHash gethcommon.Hash field to TxTraceResult (with JSON tag) and imported gethcommon.
Tests / Test entry rename
x/evm/evmstate/keeper_test.go
Renamed top-level test function TestTestEvmState.

Sequence Diagram

sequenceDiagram
    participant Client
    participant RPC as "eth/rpc Handler"
    participant GRPC as "evmstate gRPC TraceBlock"
    participant Tracer as "Native Tracer"

    Client->>RPC: debug_traceBlockByNumber(req with timeout)
    RPC->>GRPC: TraceBlock(ctx, blockNum)
    GRPC->>Tracer: execute traces per tx
    alt Native tracer error (e.g., timeout)
        Tracer-->>GRPC: error (includes tx context)
        GRPC-->>RPC: return formatted error ("txHash @ blockHeight: ...")
        RPC-->>Client: JSON-RPC top-level error (result = null, error set)
    else Success
        Tracer-->>GRPC: []TxTraceResult (each includes TxHash)
        GRPC-->>RPC: traced results
        RPC-->>Client: JSON-RPC result array
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

x: evm

Suggested reviewers

  • onikonychev
  • k-yang

Poem

🐰 A tiny hop, a tracing tweak,
Hashes now follow each rabbit's streak,
Errors bubble up where they belong,
Zero-gas names sing a clearer song,
Tests now catch the timeout's gong.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title directly addresses the main objective of the changeset: handling native tracer errors with JSON-RPC compliance for the debug_traceBlockByNumber RPC method. The title clearly identifies the primary change (native tracer error handling and JSON-RPC error formatting) and references the linked issue #2400 that motivated this work. While the phrasing is slightly awkward grammatically ("handle native tracer errors JSON-RPC errors"), it is specific enough that a teammate reviewing the commit history would understand the core purpose is fixing error response formatting for block tracing operations.
Linked Issues Check ✅ Passed The code changes directly address the core requirements from linked issue #2400. The PR implements JSON-RPC 2.0-compliant error formatting for debug_traceBlockByNumber by returning top-level error objects with result: null instead of embedding errors within result arrays, as evidenced by changes to eth/rpc/rpcapi/api_debug.go and x/evm/evmstate/grpc_query.go. The TraceBlock implementation now follows native tracer semantics where any transaction trace failure causes the entire RPC call to fail [#2400], and test updates in tracing_test.go validate this error-handling behavior with timeout test cases. The TxHash field added to TxTraceResult in x/evm/vmtracer.go supports proper transaction identification in trace outputs.
Out of Scope Changes Check ✅ Passed All code changes in the pull request remain within scope of the stated objectives. The core trace error handling changes (api_debug.go, tracing.go, grpc_query.go, vmtracer.go) directly support JSON-RPC compliance and native tracer parity for #2400. The ante decorator rename from AnteDecSaiOracle to AnteDecZeroGasActors and associated querier helper (app/ante.go, app/ante/fixed_gas.go, x/sudo/keeper/querier.go) are explicitly mentioned in the PR description as part of the "Naming and Sudo helpers" improvements. Test updates (tracing_test.go, grpc_query_test.go, keeper_test.go) validate the new behavior and naming conventions, while CHANGELOG.md documents the fix. No unrelated or unnecessary changes are present.
Description Check ✅ Passed The pull request description substantially exceeds the minimal template requirements by providing a comprehensive Purpose/Abstract section, properly closing the linked issue (#2400), and including detailed subsections on Changes, Added/Updated Tests, and DX improvements. The author clearly explains the JSON-RPC compliance objectives, native tracer parity rationale, and implementation details including the transition from embedded errors to top-level error responses. All required information from the template is present and contextually enhanced with substantial additional detail about the rationale and scope of changes.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request primarily focuses on aligning the behavior of debug_traceBlockByNumber with Geth's native tracers by ensuring that an error in tracing a single transaction fails the entire block trace. This is a significant improvement for consistency. The PR also includes excellent refactoring of the EVM tests to make them more robust and less brittle, moving from string comparisons to structured data validation. Additionally, there are several good cleanups, such as renaming AnteDecSaiOracle to AnteDecZeroGasActors for clarity and improving the ante handler logic. However, I've identified a critical issue where an ignored error can lead to a panic, along with a couple of medium-severity suggestions to improve test code maintainability and consistency.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (7)
eth/rpc/rpcapi/api_debug.go (1)

88-90: Preserve error cause and keep height as a structured log field.

Use %w and log height separately to maintain traceability and machine-parsable context.

-    err = fmt.Errorf("%s { blockHeight: %d }", err, height)
-    a.logger.Debug("get block failed", "error", err.Error())
+    err = fmt.Errorf("get block at height %d: %w", height, err)
+    a.logger.Debug("get block failed", "height", height, "error", err.Error())
CHANGELOG.md (1)

58-59: Tiny grammar nit for clarity.
“handle native tracer errors JSON-RPC errors” → “handle native tracer errors as JSON-RPC errors”.

eth/rpc/rpcapi/tracing.go (1)

145-147: Good: empty slice on zero txs; minor log fix.

  • Returning [] when txs=0 aligns with common RPC behavior. LGTM.
  • Prefer using the loop variable for hash to avoid relying on index.
-    for i, blockTx := range blockTxs {
+    for _, blockTx := range blockTxs {
-        decodedTx, err := txDecoder(blockTx)
+        decodedTx, err := txDecoder(blockTx)
         if err != nil {
-            b.logger.Error("failed to decode transaction", "hash", blockTxs[i].Hash(), "error", err.Error())
+            b.logger.Error("failed to decode transaction", "hash", blockTx.Hash(), "error", err.Error())
             continue
         }

Note: Consider unmarshalling into a nil slice (var decodedResults []*evm.TxTraceResult) rather than a pre-sized slice to avoid confusion about lengths.

Also applies to: 156-171

x/sudo/keeper/querier.go (1)

26-28: Accessor looks good; add brief docstring for public method.
Helps discoverability in IDEs.

+// GetZeroGasActors returns the configured zero-gas actors or the default value if unset.
 func (k Keeper) GetZeroGasActors(ctx sdk.Context) sudo.ZeroGasActors {
     return k.ZeroGasActors.GetOr(ctx, sudo.DefaultZeroGasActors())
 }
eth/rpc/rpcapi/tracing_test.go (1)

98-99: Great coverage; also assert JSON-RPC error details.

When wantErr=true, assert the client returns a JSON-RPC error (code/message), not just any error.

@@
 import (
+    "errors"
@@
-    "github.com/NibiruChain/nibiru/v2/eth/rpc"
+    "github.com/NibiruChain/nibiru/v2/eth/rpc"
+    gethrpc "github.com/ethereum/go-ethereum/rpc"
 )
@@
-                if tc.wantErr {
-                    s.Require().Error(err, tc.wantErr)
-                    return
-                }
+                if tc.wantErr {
+                    s.Require().Error(err)
+                    var rpcErr *gethrpc.Error
+                    if errors.As(err, &rpcErr) {
+                        s.NotZero(rpcErr.ErrorCode())
+                        s.Contains(rpcErr.Error(), "timeout")
+                    }
+                    return
+                }
@@
-                if tc.wantErr {
-                    s.Require().Error(err)
-                    return
-                }
+                if tc.wantErr {
+                    s.Require().Error(err)
+                    var rpcErr *gethrpc.Error
+                    if errors.As(err, &rpcErr) {
+                        s.NotZero(rpcErr.ErrorCode())
+                        s.Contains(rpcErr.Error(), "timeout")
+                    }
+                    return
+                }

Optional: add an HTTP-level assertion that "result" is null when error, if your test harness exposes raw JSON.

Also applies to: 128-139, 151-155, 166-171

x/evm/evmstate/grpc_query.go (1)

706-709: Comment typo.

-    // parallel process that generates statesin one thread and traces txs in
+    // parallel process that generates states in one thread and traces txs in
x/evm/evmstate/grpc_query_test.go (1)

855-888: Robust field-by-field comparison with address normalization.

The new comparison logic is significantly more resilient:

  • Unmarshals response into structured map
  • Normalizes addresses to handle EIP-55 mixed-case vs lowercase
  • Handles hex values with case-insensitive comparison

This aligns with the PR objective to make test comparisons less brittle.

Consider adding validation that no unexpected fields are present:

 			}
+			// Optional: Verify no unexpected fields
+			for k := range gotTraceRes {
+				if _, expected := wantResp[k]; !expected {
+					s.T().Logf("Unexpected field in trace result: %s = %v", k, gotTraceRes[k])
+				}
+			}
 		})
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6316bcd and 64d81ca.

📒 Files selected for processing (11)
  • CHANGELOG.md (1 hunks)
  • app/ante.go (1 hunks)
  • app/ante/fixed_gas.go (2 hunks)
  • eth/rpc/rpcapi/api_debug.go (1 hunks)
  • eth/rpc/rpcapi/api_eth_test.go (1 hunks)
  • eth/rpc/rpcapi/tracing.go (2 hunks)
  • eth/rpc/rpcapi/tracing_test.go (4 hunks)
  • x/evm/evmstate/grpc_query.go (1 hunks)
  • x/evm/evmstate/grpc_query_test.go (10 hunks)
  • x/evm/evmstate/keeper_test.go (1 hunks)
  • x/sudo/keeper/querier.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
x/sudo/keeper/querier.go (4)
x/sudo/keeper/keeper.go (1)
  • Keeper (16-19)
x/sudo/state.pb.go (3)
  • ZeroGasActors (136-143)
  • ZeroGasActors (147-147)
  • ZeroGasActors (148-150)
x/sudo/msgs.go (1)
  • DefaultZeroGasActors (134-139)
x/sudo/query.pb.go (3)
  • QueryZeroGasActorsResponse (156-158)
  • QueryZeroGasActorsResponse (162-162)
  • QueryZeroGasActorsResponse (163-165)
app/ante.go (2)
app/ante/fixed_gas.go (1)
  • AnteDecZeroGasActors (67-69)
app/keepers/all_keepers.go (1)
  • PublicKeepers (35-75)
x/evm/evmstate/grpc_query.go (1)
x/evm/vmtracer.go (1)
  • TxTraceResult (70-73)
app/ante/fixed_gas.go (1)
app/keepers/all_keepers.go (1)
  • PublicKeepers (35-75)
x/evm/evmstate/grpc_query_test.go (3)
x/evm/evmtest/test_deps.go (1)
  • TestDeps (27-35)
x/evm/evmtest/tx.go (1)
  • DeployAndExecuteERC20Transfer (103-145)
x/evm/vmtracer.go (1)
  • TxTraceResult (70-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: heavy-tests
  • GitHub Check: unit-tests
  • GitHub Check: build
  • GitHub Check: e2e-evm
  • GitHub Check: lint
  • GitHub Check: proto-gen-rs
🔇 Additional comments (11)
x/evm/evmstate/keeper_test.go (1)

21-24: LGTM: clearer test entry point name.

eth/rpc/rpcapi/api_eth_test.go (1)

311-318: LGTM: align EstimateGas behavior with geth for tiny wei values.

x/sudo/keeper/querier.go (1)

35-36: LGTM: use centralized accessor in query.

x/evm/evmstate/grpc_query_test.go (5)

810-815: Good defensive programming with nil check.

Extracting toAddr from the transaction and verifying it's not nil before using it prevents potential panics.


962-983: Proper handling of nested trace result structure.

The code correctly:

  • Validates JSON structure
  • Unmarshals into []evm.TxTraceResult
  • Checks array is non-empty before accessing first element
  • Extracts the nested Result field
  • Provides clear error messages at each step

984-997: Consistent field comparison logic with TraceTx.

The comparison logic mirrors TestTraceTx with proper address normalization for hex values. This consistency improves maintainability.


1077-1082: LGTM - cleaner test assertions.

The assertion style updates provide better error context without changing test logic.


51-83: The gas values are plausible but require manual verification against actual contract execution.

The two helper functions serve different purposes and are NOT contradictory:

  • TraceOutputERC20Transfer() (line 88): opcode tracer format with structLogs, gas=35062
  • TraceResCallTracer_ERC20Transfer() (line 51): callTracer format without structLogs, gas=51250/gasUsed=34150

These are fundamentally different tracers producing different outputs for the same transaction.

For the new TraceResCallTracer_ERC20Transfer() function:

  • gas: 51250 is a reasonable call allocation for ERC20 transfers (within typical 45k-105k range)
  • gasUsed: 34150 correctly shows actual consumption < allocation (67% usage ratio is realistic)
  • TxGas (21000) for Nibi transfer is correct (verified as standard Ethereum constant)

However, the specific values (51250, 34150) depend on your ERC20 contract implementation and cannot be definitively verified without running traces on actual execution. These appear to be measured/captured values rather than derived constants, so they should be validated against your specific contract behavior.

app/ante.go (1)

50-50: LGTM - clearer naming for ante decorator.

The rename from AnteDecSaiOracle to AnteDecZeroGasActors better describes the decorator's purpose: applying zero gas costs to whitelisted actor transactions.

app/ante/fixed_gas.go (2)

64-69: Clearer documentation and naming.

The updated comments and type name better describe the decorator's purpose: handling zero-gas costs for whitelisted contract executions by specific senders.


77-77: Code change verified and approved.

The method GetZeroGasActors is properly defined in x/sudo/keeper/querier.go:26 with signature func (k Keeper) GetZeroGasActors(ctx sdk.Context) sudo.ZeroGasActors. The call at line 77 is correct and matches the method definition perfectly.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 64d81ca and cd296f8.

📒 Files selected for processing (1)
  • x/evm/vmtracer.go (2 hunks)
🔇 Additional comments (1)
x/evm/vmtracer.go (1)

7-7: LGTM: Import alias is appropriate.

The gethcommon alias clearly distinguishes the ethereum common package and is necessary for the new TxHash field.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
x/evm/evmstate/grpc_query_test.go (1)

1014-1027: Address comparison differs from TraceTx.

The address field comparison here uses only lowercase normalization (lines 1020-1024), while TraceTx uses gethcommon.HexToAddress for proper address comparison (TraceTx lines 902-909). For consistency and correctness, consider adopting the TraceTx approach.

Apply this pattern from TraceTx:

 			for k, v := range wantResp {
 				gotV, ok := gotTraceRes[k]
 				s.Require().Truef(ok,
 					"expect result.%s field to be present: { got: %s, want: %s }",
 					k, gotTraceRes, wantResp,
 				)
-				if strings.HasPrefix(gotV, "0x") {
-					// Hex addresses aren't case sensitive. Normalize to lower
-					// case for
-					v = strings.ToLower(v)
-					gotV = strings.ToLower(gotV)
+				switch k {
+				case "from", "to":
+					// Compare as addresses (case-insensitive, byte-equal)
+					s.Equalf(
+						gethcommon.HexToAddress(v),
+						gethcommon.HexToAddress(gotV),
+						`mismatch in trace result { key: %s, testCase: "%s" }`, k, tc.name,
+					)
+				default:
+					// If it's hex, ignore case; else exact
+					if strings.HasPrefix(gotV, "0x") {
+						v = strings.ToLower(v)
+						gotV = strings.ToLower(gotV)
+					}
+					s.Equalf(v, gotV, `mismatch in trace result { key: %s, testCase: "%s" }`, k, tc.name)
 				}
-				s.Equalf(v, gotV, `mismatch in trace result { key: %s, testCase: "%s" }`, k, tc.name)
 			}
🧹 Nitpick comments (1)
x/evm/evmstate/grpc_query.go (1)

711-729: Consider populating TxHash field in TxTraceResult.

The changes correctly implement the native tracer behavior by failing the entire block trace on any transaction error. However, based on the TxTraceResult struct definition (which includes a TxHash field), consider setting it for richer trace data:

 	for i, tx := range req.Txs {
 		result := evm.TxTraceResult{}
 		ethTx := tx.AsTransaction()
+		result.TxHash = ethTx.Hash()
 		txConfig.TxHash = ethTx.Hash()
 		txConfig.TxIndex = uint(i)

This would align with the test expectations and provide more complete trace information to clients.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd296f8 and 1f3fff5.

📒 Files selected for processing (3)
  • CHANGELOG.md (1 hunks)
  • x/evm/evmstate/grpc_query.go (1 hunks)
  • x/evm/evmstate/grpc_query_test.go (10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.

Applied to files:

  • x/evm/evmstate/grpc_query_test.go
📚 Learning: 2025-09-16T00:14:44.279Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).

Applied to files:

  • x/evm/evmstate/grpc_query.go
📚 Learning: 2024-10-13T05:48:51.771Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.

Applied to files:

  • x/evm/evmstate/grpc_query.go
🧬 Code graph analysis (2)
x/evm/evmstate/grpc_query_test.go (4)
x/evm/evmtest/test_deps.go (1)
  • TestDeps (27-35)
x/evm/evmtest/tx.go (1)
  • DeployAndExecuteERC20Transfer (103-145)
x/evm/query.pb.go (6)
  • QueryTraceTxRequest (830-850)
  • QueryTraceTxRequest (854-854)
  • QueryTraceTxRequest (855-857)
  • QueryTraceBlockRequest (995-1012)
  • QueryTraceBlockRequest (1016-1016)
  • QueryTraceBlockRequest (1017-1019)
x/evm/vmtracer.go (1)
  • TxTraceResult (71-75)
x/evm/evmstate/grpc_query.go (1)
x/evm/vmtracer.go (1)
  • TxTraceResult (71-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: heavy-tests
  • GitHub Check: lint
  • GitHub Check: unit-tests
  • GitHub Check: build
  • GitHub Check: e2e-evm
  • GitHub Check: proto-gen-rs
🔇 Additional comments (6)
x/evm/evmstate/grpc_query.go (2)

694-710: LGTM - Clear documentation and correct initialization.

The comments effectively explain why Nibiru uses native tracers and how this affects error propagation semantics. The pre-allocated results slice with fixed length correctly supports the index-assignment pattern used later at line 728.


716-719: Acknowledged - TransactionToMessage error handling.

The ignored error here was previously discussed in past reviews. The maintainer confirmed that msg is guaranteed to be non-nil from core.TransactionToMessage for tracing queries.

x/evm/evmstate/grpc_query_test.go (4)

52-84: LGTM - Well-structured test helpers.

The helper functions effectively reduce test duplication and provide clear expected trace outputs. The parameterization by addresses makes them flexible for different test scenarios.

Note: The hardcoded gas values (51_250, 34_150, 21000) may require updates if gas costs change in future protocol upgrades.


823-918: LGTM - Robust field-by-field comparison.

The test updates correctly validate trace results with proper address normalization (lines 902-909) and hex value case-insensitivity (lines 910-917). The structured comparison approach is more maintainable than string matching.


1006-1012: Acknowledged - Marshal-then-unmarshal pattern.

The marshal-unmarshal pattern for the Result field is necessary because it has type any. This was previously discussed in past reviews where the maintainer explained this approach is required.


1107-1111: LGTM - Improved test assertions.

The changes make the test assertions more explicit and provide better error messages when failures occur.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
x/evm/vmtracer.go (1)

70-77: Adding TxHash to TxTraceResult — good for per-tx identification.

JSON tags look right; TxHash non-omitempty ensures presence. Optional: tighten Result to *json.RawMessage to avoid accidental non-JSONable assignments.

+import "encoding/json"
 
 type TxTraceResult struct {
 	TxHash gethcommon.Hash `json:"txHash"`           // transaction hash
-	Result any             `json:"result,omitempty"` // Trace results produced by the tracer
+	Result *json.RawMessage `json:"result,omitempty"` // Trace results produced by the tracer
 	Error  string          `json:"error,omitempty"`  // Trace failure produced by the tracer
 }
x/evm/evmstate/grpc_query.go (2)

694-705: Minor comment fixes for clarity.

Typo and phrasing; suggest tightening.

-// JS tracers (geth only) have high overhead. Tracing for them runs a
-// parallel process that generates statesin one thread and traces txs in
-// separate worker threads. JS tracers store tracing errors for each tx as
-// fields of the returned trace result instead of failing the query.
+// JS tracers (geth only) have high overhead. Tracing runs a parallel process
+// that generates states in one thread and traces txs in worker threads.
+// JS tracers record per‑tx errors in the result instead of failing the query.

714-729: TxHash population resolved; preserve gRPC status codes on trace failure.

Setting result.TxHash and txConfig.TxHash looks good and addresses prior gap. However, wrapping a gRPC status error with fmt.Errorf drops the original status code. Either return the original err or re-wrap preserving its code.

Option A (simplest):

-        return nil, fmt.Errorf("trace tx error { txhash: %s, blockHeight: %d }: %w", ethTx.Hash().Hex(), ctx.BlockHeight(), err)
+        return nil, err

Option B (preserve code, keep context):

+        st, ok := grpcstatus.FromError(err)
+        if ok {
+            return nil, grpcstatus.Errorf(st.Code(),
+                "trace tx error { txhash: %s, blockHeight: %d }: %s",
+                ethTx.Hash().Hex(), ctx.BlockHeight(), st.Message())
+        }
-        return nil, fmt.Errorf("trace tx error { txhash: %s, blockHeight: %d }: %w", ethTx.Hash().Hex(), ctx.BlockHeight(), err)
+        return nil, grpcstatus.Errorf(grpccodes.Internal,
+            "trace tx error { txhash: %s, blockHeight: %d }: %s",
+            ethTx.Hash().Hex(), ctx.BlockHeight(), err.Error())
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1f3fff5 and c486bde.

📒 Files selected for processing (2)
  • x/evm/evmstate/grpc_query.go (1 hunks)
  • x/evm/vmtracer.go (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-16T00:14:44.279Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).

Applied to files:

  • x/evm/evmstate/grpc_query.go
📚 Learning: 2024-10-13T05:48:51.771Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.

Applied to files:

  • x/evm/evmstate/grpc_query.go
🧬 Code graph analysis (1)
x/evm/evmstate/grpc_query.go (1)
x/evm/vmtracer.go (1)
  • TxTraceResult (73-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: heavy-tests
  • GitHub Check: lint
  • GitHub Check: e2e-evm
  • GitHub Check: proto-gen-rs
  • GitHub Check: unit-tests
  • GitHub Check: build
🔇 Additional comments (2)
x/evm/vmtracer.go (1)

7-7: Import alias addition is fine.

Alias gethcommon is consistent with usage below.

x/evm/evmstate/grpc_query.go (1)

706-710: Pre-sized results slice — correct choice.

Length-based allocation avoids index panics and preserves deterministic ordering.

@Unique-Divine Unique-Divine enabled auto-merge (squash) October 23, 2025 19:55
@Unique-Divine Unique-Divine merged commit e44cbc5 into main Oct 23, 2025
10 of 11 checks passed
@Unique-Divine Unique-Divine deleted the ud/trace-block branch October 23, 2025 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Misformatted debug_TraceBlockByNumber output with callTracer

2 participants